Skip to content

Conversation

@Patrick-DS
Copy link

I would actually like to do bigger changes in the repository for the functionality that I need, but I wanted to initialize the discussion with this commit because it's the safest bet and shows an example of what I want to implement.

The views have a very bulky post method, so subclassing them to try to change a little bit of functionality ends up re-creating the class almost entirely and makes subclassing useless, going against the open-closed principle. I ended up re-writing a lot of code because of that.

The .post method in ResetPasswordConfirm was very bulky and much of the functionality inside the post method made more sense as a collection of smaller methods with a more precise task, because they could be re-defined for another developer's use-case (this is my use-case; I have three user types and boolean properties attached to the user model that I can use to tell the user types apart, and I needed to re-define "eligible_for_reset"). Functionality is left unchanged but the steps can now be adjusted if a developer subclasses this view. The post method is also now much more readable.

  • The token is only used to fetch the user from the DB, so we have the "get_user_from_token" method.
  • The purpose of the try-except block for the password validation is now clearer; isolated as a method, the method validates the password and converts the Django validation error into a DRF one
  • We used a contextmanager to trigger signals instead of just writing the signals. This gives a child class more control over what to do before and after the password change, improves readability, and puts all signal triggering in one place.
  • There's an actual "reset_password" method now.

The .post method in ResetPasswordConfirm was very bulky and much of the functionality inside the post method made more sense as a collection of smaller methods with a more precise task, because they could be re-defined for another developer's use-case (this is my case). Functionality is left unchanged but the steps can now be adjusted if a developer subclasses this view. The post method is also now much more readable.

- The token is only used to fetch the user from the DB, so we have the "get_user_from_token" method. 
- The purpose of the try-except block for the password validation is now clearer; isolated as a method, the method validates the password and converts the Django validation error into a DRF one
- We used a contextmanager to trigger signals instead of just writing the signals. This gives a child class more control over what to do before and after the password change, improves readability, and puts all signal triggering in one place.
- There's an actual "reset_password" method now.
- Replaced tabs by spaces
@nezhar nezhar self-requested a review August 18, 2021 17:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants